Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Git application #184

Merged
merged 85 commits into from
Dec 28, 2018
Merged

Git application #184

merged 85 commits into from
Dec 28, 2018

Conversation

bsekachev
Copy link
Member

Resolved #126

@bsekachev bsekachev added the enhancement New feature or request label Nov 1, 2018
@bsekachev bsekachev force-pushed the git_application branch 2 times, most recently from 9786f7e to c47976d Compare November 22, 2018 13:39
@bsekachev bsekachev changed the title [WIP] Git application Git application Nov 22, 2018
@bsekachev bsekachev force-pushed the git_application branch 4 times, most recently from 8230872 to 74d4647 Compare November 27, 2018 06:26
cvat/apps/git/git.py Show resolved Hide resolved
cvat/apps/git/git.py Show resolved Hide resolved
@nmanovic
Copy link
Contributor

nmanovic commented Dec 3, 2018

Great job! But let's fix a couple of issues:

  • Let's rename "Git URL" to "Dataset repository"
  • Let's don't have "Git Path" as a separate field. I propose:
    Dataset repository [ git repository url ] [ relative path to file ]
  • Let's add popup hints for "git url" and "git path"
  • Error handling if ssh key isn't added should be better. It will be a usual mistake. Need to ask the user to add ssh key into git server explicitly and probably show the key (otherwise the user should go into container and copy the key)

@nmanovic
Copy link
Contributor

nmanovic commented Dec 3, 2018

  • User for commit should be changed (now it is ExampleOrgz)
  • Sync status should be more visible in dashboard. I propose colored background for task item (red - unsync, yellow - sync but not merged, green - merged)

@nmanovic
Copy link
Contributor

nmanovic commented Dec 3, 2018

  • diff file was committed into my repository. Is it expected?

@nmanovic
Copy link
Contributor

nmanovic commented Dec 3, 2018

  • git url and path isn't dumped into the xml file itself.

@nmanovic
Copy link
Contributor

nmanovic commented Dec 3, 2018

  • Need to provide good default value for git path (e.g. <dump file name>.xml)

@bsekachev
Copy link
Member Author

@nmanovic

"Error handling if ssh key isn't added should be better. It will be a usual mistake. Need to ask the user to add ssh key into git server explicitly"
Interaction with an user during building is actually good idea?

@bsekachev
Copy link
Member Author

bsekachev commented Dec 3, 2018

@nmanovic
"diff file was committed into my repository. Is it expected?"
changelog_*.diff is commited to repos. It is expected. Other diffs files stores outside repository directory.

@bsekachev
Copy link
Member Author

@nmanovic
"Sync status should be more visible in dashboard. I propose colored background for task item (red - unsync, yellow - sync but not merged, green - merged)"
It is very long for loading because each "get_remote_status" request does request to git server (1-3 sec). I have no idea how implement it effectively yet.

@bsekachev
Copy link
Member Author

@nmanovic
Other comments have been implemented

@nmanovic
Copy link
Contributor

nmanovic commented Dec 3, 2018

Interaction with an user during building is actually good idea?

It isn't during building. It is when a user try to push something into a repository for which the user doesn't add generated ssh key. It will get some error that the repository cannot be cloned with a lot of extra stuff. I remember that it isn't so easy to understand why a repository cannot be cloned. Let's say that "Probably SSH key was not added to your profile for the cloned repository. Please add the key below and try again".

@nmanovic
Copy link
Contributor

nmanovic commented Dec 3, 2018

changelog_*.diff is commited to repos. It is expected. Other diffs files stores outside repository directory.

Why do we need it inside repository? Is it a temporary file for internal purpose?

@nmanovic nmanovic closed this Dec 3, 2018
@nmanovic nmanovic reopened this Dec 3, 2018
@nmanovic
Copy link
Contributor

nmanovic commented Dec 3, 2018

It is very long for loading because each "get_remote_status" request does request to git server (1-3 sec). I have no idea how implement it effectively yet.

Indeed the task is very important. It is really good feature. I need it to be sure that latest tasks are synchronized and it will be inconvenient to click each time on a task to understand its status.

There are several ways to implement that:

  • Just cache the latest status always and return it together with task info for dashboard. But it will necessary to click each time the task push as well. But one clicked it will be "save" state.
  • Ask users to add a webhook. I don't like the approach but probably it will work best with the approach above.
  • Implement a background task which sync the status time to time for non-merged repos and cache it. It will work best from my point of view. Also when a user press "push" button the status is cached as well.

@nmanovic
Copy link
Contributor

nmanovic commented Dec 3, 2018

@bsekachev ,

Does ssh key is regenerated each time when you re-build CVAT? If it is so need to fix that. It should be generated once as django security key. It is really a pain to add a new key each time.

@bsekachev
Copy link
Member Author

@nmanovic
"Why do we need it inside repository? Is it a temporary file for internal purpose?"
No, this contains information about changed objects and dates.

@nmanovic
Copy link
Contributor

nmanovic commented Dec 3, 2018

@bsekachev ,

Is it possible to incorporate the information into git commit message? Why do we need a separate file for that?

@bsekachev
Copy link
Member Author

@nmanovic
Please, review it again.

Dockerfile Outdated Show resolved Hide resolved
cvat/apps/git/README.md Outdated Show resolved Hide resolved
cvat/apps/git/migrations/0001_initial.py Show resolved Hide resolved
cvat/apps/git/models.py Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
ssh/README.md Outdated Show resolved Hide resolved
ssh/README.md Outdated Show resolved Hide resolved
@nmanovic nmanovic merged commit 23b6b54 into develop Dec 28, 2018
@nmanovic nmanovic deleted the git_application branch December 28, 2018 13:11
TOsmanov pushed a commit to TOsmanov/cvat that referenced this pull request Aug 23, 2021
)

* Avoid saving masks in voc when segmentation is not required
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants